Skip to content

[ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary#260618

Merged
stratoula merged 15 commits into
elastic:mainfrom
bartoval:esql-marker-cleanup-assignment
Apr 13, 2026
Merged

[ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary#260618
stratoula merged 15 commits into
elastic:mainfrom
bartoval:esql-marker-cleanup-assignment

Conversation

@bartoval
Copy link
Copy Markdown
Contributor

@bartoval bartoval commented Apr 1, 2026

Summary

Closes #215186

Disclaimer : This PR restarts following the last comment on the issue. So I’m trying to clean up the ESQL commands as a consumers, without venturing into suicide missions.


  • Fixes most of the remaining editor marker leak for incomplete assignments in autocomplete: so nested marker nodes are also removed from the AST.
  • Introduces a shared autocomplete parsing boundary to centralize query repair, parsing, cursor-context extraction, and marker cleanup.
  • Removes redundant marker-aware checks from command/runtime paths where they are no longer needed.
  • Keeps the assignment-specific fallback in getAssignmentExpressionRoot(), since incomplete RHS still needs to behave like an empty expression in cases such as RERANK ... ON col0 =.

@bartoval bartoval self-assigned this Apr 1, 2026
@bartoval bartoval added release_note:skip Skip the PR/issue when compiling release notes backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana Team:ESQL ES|QL related features in Kibana t// v9.4.0 labels Apr 1, 2026
@bartoval bartoval changed the title [ES|QL] Fix nested editor marker leaks inautocomplete [ES|QL] Fix nested editor marker leaks in autocomplete and introduce a shared autocomplete parsing boundary Apr 1, 2026
@bartoval bartoval force-pushed the esql-marker-cleanup-assignment branch from 26d90f2 to e53d9bd Compare April 1, 2026 07:05
@bartoval
Copy link
Copy Markdown
Contributor Author

bartoval commented Apr 1, 2026

I was thinking about a more ambitious long-term idea (not for this PR):

Instead of using markers and cleaning them up later in Kibana, @elastic/esql could handle incomplete input directly in the AST. For example, foo = could produce { kind: 'assignment_rhs' } instead of a marker-based node.

Using location instead of marker_esql_editor would also let us classify contexts more precisely (e.g. assignment_rhs, function_argument, list_item), so consumers don’t need to know about markers.

Before that, we should verify that parser locations are reliable with incomplete input. There are already some issues, like LIKE / NOT resolving to unknown, and trailing commas resolving only to very broad nodes.

if (Array.isArray(arg)) {
return arg.filter(isNotMarkerNodeOrArray).map(mapToNonMarkerNode);
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the missing part of the task

@@ -85,7 +84,7 @@ function getPosition(
return { position: CompletionPosition.AFTER_COMMAND };
}

const expressionRoot = prompt?.text !== EDITOR_MARKER ? prompt : undefined;
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this by hand, I found no side effects

@bartoval bartoval force-pushed the esql-marker-cleanup-assignment branch from e53d9bd to a315d3c Compare April 1, 2026 07:27
@@ -181,8 +181,7 @@ export async function autocomplete(

case 'after_where': {
const whereFn = command.args[command.args.length - 1] as ESQLFunction;
// TODO do we still need this check?
const expressionRoot = isMarkerNode(whereFn.args[1]) ? undefined : whereFn.args[1]!;
const expressionRoot = whereFn.args[1];
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did some testing by hand but the unit tests already covered it well

}

/** Parses the query and resolves the cursor context (command, option, node). */
export function getAutocompleteCursorContext(fullText: string, offset: number) {
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It’s a weak API in the sense that it’s only used in one place at runtime and in tests. I could create a separate API for findAutocompleteAstPosition, but I’d rather not introduce more APIs and risk adding confusion.

@bartoval bartoval marked this pull request as ready for review April 1, 2026 07:52
@bartoval bartoval requested a review from a team as a code owner April 1, 2026 07:52
@elasticmachine
Copy link
Copy Markdown
Contributor

Pinging @elastic/kibana-esql (Team:ESQL)

@bartoval bartoval requested a review from drewdaemon April 10, 2026 14:34
Copy link
Copy Markdown
Contributor

@drewdaemon drewdaemon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Eye candy 👏

@bartoval bartoval added v9.5.0 and removed v9.4.0 labels Apr 13, 2026
Copy link
Copy Markdown
Contributor

@stratoula stratoula left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks very nice, can you fix the regex issue that was marked? Other than that I really like this


// After `KEY BY field,`, suggest the next KEY BY field.
const KEY_BY_TRAILING_COMMA_REGEX = /\bkey\s+by(?:\s+\S+,)+\s*$/i;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartoval let's fix that otherwise when we merge an issue will be created. Let's fix it here

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had already fixed it here 42fc7a6 that comment refers to the first version

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ah nice


const functionExpression = expressionRoot as ESQLFunction;
const paramContext = buildExpressionFunctionParameterContext(functionExpression, context);
const startingNewParam = STARTING_NEW_PARAM_REGEX.test(innerText);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This regex is duplicated. in_function.ts is STARTING_NEW_PARAM_REGEX and stats/autocomplete.ts uses inline /,\s*$/

We could create a shared utility.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@bartoval bartoval enabled auto-merge (squash) April 13, 2026 16:22
@stratoula stratoula disabled auto-merge April 13, 2026 16:34
@stratoula stratoula merged commit df2e76b into elastic:main Apr 13, 2026
18 of 19 checks passed
@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Metrics [docs]

Module Count

Fewer modules leads to a faster build time

id before after diff
esql 981 982 +1

Page load bundle

Size of the bundles that are downloaded on every page load. Target size is below 100kb

id before after diff
kbnUiSharedDeps-srcJs 4.2MB 4.2MB +572.0B

History

cc @bartoval

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

backport:skip This PR does not require backporting Feature:ES|QL ES|QL related features in Kibana release_note:skip Skip the PR/issue when compiling release notes Team:ESQL ES|QL related features in Kibana t// v9.5.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[ES|QL] Improve our usage of the editor marker

6 participants